-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add boost to KerrSchild #5610
Add boost to KerrSchild #5610
Conversation
624a213
to
1366cf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work overall. Two points:
1.) I think we discussed that with a boost, KerrSchild
is technically time dependent but you have only implemented it for t=0
. It is therefore technically no longer a AnalyticSolution
. Can you make sure to document this?
2.) As we discussed, could you write a test that computes tensors like the extrinsic curvature and the spacetime metric once boosted and once unboosted and make sure they correspond to a manual transformation?
* ## Boost of the Kerr-Schild solution | ||
* | ||
* We add initial momentum to the solution by applying a Lorentz boost to the | ||
* metric. Given the form of the Kerr-Schild metric in terms of a covariant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rephrase this to something more readable? Perhaps "The Kerr-Schild metric can be expressed covariantly in terms of (...). We construct the boosted metric by boosting these object individually."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
internal_tags::x_minus_center<DataType, Frame> /*meta*/) const; | ||
|
||
void operator()(const gsl::not_null<Scalar<DataType>*> a_dot_x, | ||
const gsl::not_null<CachedBuffer*> cache, | ||
void operator()(gsl::not_null<Scalar<DataType>*> a_dot_x, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you delete all these const
specifiers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy was complaining. Said only has effect in the cpp
@@ -391,8 +450,7 @@ void KerrSchild::IntermediateComputer<DataType, Frame>::operator()( | |||
const auto& lapse = get(cache->get_var(*this, gr::Tags::Lapse<DataType>{})); | |||
const auto& lapse_squared = | |||
get(cache->get_var(*this, internal_tags::lapse_squared<DataType>{})); | |||
get(*deriv_lapse_multiplier) = | |||
-square(null_vector_0_) * lapse * lapse_squared; | |||
get(*deriv_lapse_multiplier) = -lapse * lapse_squared; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can you set square(null_vector_0_)
to 1 here but not other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its set to one, just that it seems not in the formula for the spatial derivative of the lapse.
template <size_t SpatialDim> | ||
tnsr::Ab<double, SpatialDim, Frame::NoFrame> lorentz_boost_matrix( | ||
const std::array<double, SpatialDim>& velocity) { | ||
tnsr::I<double, SpatialDim, Frame::NoFrame> velocity_as_tensor{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tnsr::I<double, SpatialDim, Frame::NoFrame> velocity_as_tensor(velocity)
should work I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Seems to work
/*! | ||
* \ingroup SpecialRelativityGroup | ||
* \brief Apply a Lorentz boost to the spatial part of a one form. | ||
* \details The zero component of the one form is also specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "This requires passing the 0th component of the one form as an additional argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -58,6 +59,80 @@ void lorentz_boost_matrix( | |||
(*boost_matrix).get(i + 1, i + 1) += 1.0; | |||
} | |||
} | |||
|
|||
template <size_t SpatialDim> | |||
tnsr::Ab<double, SpatialDim, Frame::NoFrame> lorentz_boost_matrix( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make this a symmetric tensor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I? This has one upper and one lower index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not currently supported.
} | ||
const auto boost_matrix_1 = lorentz_boost_matrix(velocity_first_index); | ||
const auto boost_matrix_2 = lorentz_boost_matrix(velocity_second_index); | ||
for (size_t i = 0; i < SpatialDim + 1; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] tenex::evaluate<ti::a, ti::b>(result, boost_matrix_1(ti::C, ti::a) * boost_matrix_2(ti::D, ti::b) * tensor(ti::c, ti::d)
is quite concise.
* Note that the Lorentz matrix is symmetric. | ||
*/ | ||
template <typename DataType, size_t SpatialDim, typename Frame> | ||
void lorentz_boost( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, there would be one implementation that works for a generic tensor type so do not have all of these overloads. If you think that would be overkill for now, maybe just add a comment that this can be done in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I wouldn't want to delay this anymore. Added comment.
@@ -44,7 +44,7 @@ void compare_different_wrapped_solutions(const double mass, | |||
template <typename SolutionType> | |||
void test_copy_and_move(const SolutionType& solution) { | |||
test_copy_semantics(solution); | |||
auto solution_copy = solution; | |||
const auto& solution_copy = solution; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just a reference now. Can you just pass in solution
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -426,5 +426,55 @@ void verify_consistency(const Solution& solution, const double time, | |||
derivative_delta), | |||
derivative_approx); | |||
} | |||
|
|||
/// Check the consistency of dependent quantities returned by a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependent on what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On other metric quantities. Changed doc.
1366cf9
to
b40a32d
Compare
eafc3ec
to
b2532c8
Compare
@nikwit I've added one more test to manually boost the spacetime metric and check that I get the same as the implementation. Not entirely sure how to do it for the extrinsic curvature. I think I need to upgrade it to 4d tensor (not sure about this) then boost it and compare a 4d tensor obtained from the implementation. |
@nikwit was suggesting moving the KerrBoost to AnalyticData instead of AnalyticSolutions while I suggest leaving it here and add a warning in the doc. The reasoning is mainly because the Kerr boost as it is currently implemented (and as it is implemented in SpEC) is only usable for initial data and some assumptions are being made. For instance, we are not coding up the time dependence at all (the solution now is both function of position and time since its moving, but we set t = 0), and assumptions on the time derivative of the lapse and shift (which will be rewritten anyway when evolved). @nilsvu any thoughts on this? |
@geoffrey4444 Could you review this one too, since you wrote the Lorentz transformations here and boosts in SpEC? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please keep this class in AnalyticSolution
with the warning in the docs.
src/PointwiseFunctions/AnalyticSolutions/GeneralRelativity/KerrSchild.hpp
Outdated
Show resolved
Hide resolved
* metric. Since the Kerr-Schild metric can be expressed covariantly in terms of | ||
* the Minkowski metric, a scalar function and a one form, we constructing the | ||
* metric by boosting the these objects individually. Notice that we also need | ||
* to appropriately boost the coordinates to the boosted frame. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we're evaluating the boosted Kerr-Schild metric in the boosted frame, meaning that in these coordinates the black hole does not move, correct? So we inverse-boost the input coordinates to get to the unboosted frame, evaluate Kerr-Schild quantities in the unboosted frame, then boost these quantities. Please clarify the docs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will clarify. I got confused about it myself at some point. We start with coordinates in the frame where the black hole is moving. Then boost them to the frame where the black hole is at rest, and where we can construct the stationary solution. Then we boost all covariant quantities back to the frame where it is moving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, for initial data I thought we're solving in a frame where the BHs are at rest.. @geoffrey4444 can you clarify how this works in SpEC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked about this in the meeting today. I think you're correct. For initial data we go into a rotating frame that gets rid of the translation but keeps the Lorentz contraction.
* \warning While technically the boosted Kerr-Schild metric is dependent on | ||
* both the time and space coordinates, we have implemented it only at $t = 0$ | ||
* as in SpEC. Therefore it is technically not an analytic solution and should | ||
* not be used to compute errors with respect to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution should be stationary in the boosted frame; so shouldn't all time derivatives be zero and this be a solution at any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it is stationary in the rest frame, which is not the frame we want. In the frame we want, the black hole is moving and so the metric will be dependent on t and x. I didn't code this t dependence since I didn't see this in SpEC --it seems to me it should only enter in the function that computes the radius to the center.
I also didn't code the time derivatives of the lapse and shift in the frame where the black hole is moving (which are zero in the rest frame of course) but left them as zero. This should be fine because I think they are replaced by the gauge functions we use to evolve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think you're correct. Let me look at the SpEC implementation too.
@@ -58,6 +59,80 @@ void lorentz_boost_matrix( | |||
(*boost_matrix).get(i + 1, i + 1) += 1.0; | |||
} | |||
} | |||
|
|||
template <size_t SpatialDim> | |||
tnsr::Ab<double, SpatialDim, Frame::NoFrame> lorentz_boost_matrix( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not currently supported.
/// @{ | ||
/*! | ||
* \ingroup SpecialRelativityGroup | ||
* \brief Apply a Lorentz boost to the spatial part of a one form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument type is a vector (tnsr::I
), so I also think this documentation should read vector
. Refer to the docs of lorentz_boost_matrix
above for details and math. I also find those docs confusing though. I believe they say that
- The docs and argument for this function should read
vector
(they boost a vector) - The function for the one-form below should apply the inverse Lorentz transformation (by inverting the sign of the velocity). This means you add a minus sign to the function below and remove the minus signs from where the function is used in Kerr-Schild. Make sure to clarify this in the docs of the function.
@geoffrey4444 can you confirm or clarify this?
tests/Unit/PointwiseFunctions/AnalyticSolutions/GeneralRelativity/Test_KerrSchild.cpp
Outdated
Show resolved
Hide resolved
@@ -366,13 +380,116 @@ void test_zero_spin_optimization(const DataType& used_for_size) { | |||
}); | |||
} | |||
|
|||
template <typename Frame, typename DataType> | |||
void test_boosted_for_zero_velocity(const DataType& used_for_size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test!
// Need to evaluate at the boosted coordinates | ||
// Boost coordinates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the unboosted coords, no? (related to comments above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the terminology ''boosted/unboosted'' is a bit confusing. Rest frame (of the BH) coords I would say.
|
||
template <typename Frame, typename DataType> | ||
void test_boosted_spacetime_metric(const DataType& used_for_size) { | ||
// Test that the extrinsic curvature of the boosted solution indeed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extrinsic curvature -> spacetime metric? Also test extrinsic curvature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet sure how to do it. I think I would need to elevate the extrinsic curvature to a 4d tensor in both frames.
@@ -86,3 +167,9 @@ SPECTRE_TEST_CASE( | |||
d = large_velocity_squared; | |||
CHECK_FOR_DOUBLES(test_lorentz_boost_matrix_analytic, (1, 2, 3)); | |||
} | |||
|
|||
SPECTRE_TEST_CASE("Unit.PointwiseFunctions.SpecialRelativity.LorentzBoost", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
combine with test case above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combined
I'll try but really swamped this week. |
@nilsvu I have rebased this and added an assert that would throw an error if we call a tag I can't compute for the boost. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nilsvu does this look ready to go to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for fixup this up. Pleas squash your commits. Also to keep the git blame record straight, could you add this to your commit message:
# Two empty lines
Co-authored-by: Nils Vu <nilsvu@caltech.edu>
It's two empty lines, no |
Add more Lorentz boost functions Add instantiation of time deriv of spatial metric Add boosted Kerr initial data Expand doc Fix initialization of lorentz boost matrix Remove unused reference Add test of boosted spacetime metric Throw error for unsupported tag Co-authored-by: Nils Vu <nilsvu@caltech.edu>
Oh, sure |
Thank you for getting this done @guilara ! |
Thank you for all the detailed help with it |
Proposed changes
Add a boost to the KerrSchild solution. This is done by boosting the coordinates, null form and derivatives of the H function defining the KerrSchild solution. (For the case when there is a boost the solution depends on time as well, but we don't code up the time dependence of the coordinates for simplicity.)
Evolving this solution will require time dependent translation maps to keep the excision surface inside the apparent horizon (#5576)
This reuses items from develop...nilsvu:spectre:kerr_boost
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments